From: Mark Jamsek Subject: Re: merge chokes, creates bogus conflicts To: Christian Weisgerber , gameoftrees@openbsd.org Date: Sun, 19 Feb 2023 01:57:23 +1100 On 23-02-18 02:47PM, Stefan Sperling wrote: > On Sat, Feb 18, 2023 at 11:50:16PM +1100, Mark Jamsek wrote: > > There was a problem I found in the original -C diff where after forcing > > a commit with unresolved conflicts using 'got commit -C', the diff of > > that commit with 'got diff -c', 'got log -p', or tog, presented > > a completely empty diff! > > Oops. The test didn't catch this, so I guess it should be expanded? Good idea! I've added this to the test. > > Another artifact of this conflict > > management tweak that I'm not certain about is the change in update.sh > > where a binary file that already had conflict markers is now reported as > > "M foo" instead of "C foo", but I think this comports with the desired > > behaviour? > > Yes, it is in line with the new expected behaviour. Great! I confess I spent way too much time tonight considering whether it was or wasn't, and in the end decided it was too :) > > C conflicted/file/path > > resolve conflicts or use -C to commit with unresolved conflicts > > got: cannot commit file in conflicted status > > > > That said, I really don't mind whether we keep or nix it, or report > > something else. > > I would rather avoid suggesting use of -C in error messages. > > This could lead to people using -C as a lazy alternative instead > of resolving conflicts. It is better to bury the existence of -C > in the man page where it is documented with an appropriate warning. > It will be discovered by users who have a real need for it and search > the documentation for clues. > > If we are going to adjust the message then I would suggest we change > the error message to explcitly suggest that conflicts must be resolved. > This would guide users towards the correct way out. > > got: cannot commit file in conflicted status; conflicts must be resolved before changes can be committed Agree on all counts! I've left the original error message too. I think it is already clear that conflicts must be resolved before committing. And we're not going to further advertise the -C workaround. > > > +Conflicted files committed with > > +.Fl C > > +will always appear to be in conflicted status once modified in a work tree. > > +This prevents automatic merging of changes to such files during > > +.Cm got update , > > +.Cm got rebase , > > +.Cm got histedit , > > +.Cm got merge , > > +.Cm got cherrypick , > > +.Cm got backout , > > +and > > +.Cm got patch . > > The sentence above is now wrong and should be deleted. Oops, I misunderstood this: I thought it applied to the fact that he/mg/rb won't automatically continue with conflicts unless -C is used (i.e., -C -c). I missed the fact this sentence is about already versioned content with conflicts. Thank you! I've dropped it. > > If merge conflicts occur, the rebase operation is interrupted and may > > be continued once conflicts have been resolved. > > +If for some legitimate reason the conflicts cannot be resolved, the > > +.Fl C > > +option can be used to allow the rebase operation to continue despite > > +unresolved conflicts. > > +Such cases are exceedingly rare and should only be used as a last resort. > > I would drop the above parapraph to avoid promoting use of -C, and > also drop it from the histedit and merge sections. > > It is technically correct to document all possible features and > behaviours. But the more we advertise this particular feature the > higher the probability becomes that -C will be misused. I went back and forth on this myself and ultimately decided to add it--but I'll defer to your better judgement on this and drop it. > > +.It Fl C > > +Allow a rebase operation to continue with files in conflicted status. > > +This option can only be used with the > > +.Fl c > > +option. > > Let's try to discourage use a bit more: > > This option should generally be avoided, and can only be used with the > .Fl c > option. > > Same for histedit and merge. Good idea. Done! > > done: > > + if (error && error->code == GOT_ERR_COMMIT_CONFLICT) > > + printf("resolve conflicts or use -C to commit with " > > + "unresolved conflicts\n"); > > I would drop the above chunk as explained above. > > Two related nits: > > Errors should go to stderr. > > When amending an error with static information like this, consider > whether editing the string constant in lib/error.c would do the job. Good advice; I'll file this away for future reference. Thank you, and done! > > -/* Upgrade STATUS_MODIFY to STATUS_CONFLICT if a conflict marker is found. */ > > +/* > > + * Upgrade STATUS_MODIFY to STATUS_CONFLICT if a > > + * conflict marker is found in new added lines only. > > Grammar: Should this say "newly added lines"? I think I meant "new, added lines" but I actually prefer "newly", which is what I went with. Thanks! > > + err = got_diff_blob_file(blob, f1, sz1, NULL, ondisk_file, 1, sb, > > + path, GOT_DIFF_ALGORITHM_MYERS, 0, 0, 0, NULL, f); > > Myers is the right choice in this case indeed. Patience diff would > imply some additional performane overhead we don't want to pay > during the status crawl. That was my thinking :) > > + if (err) > > + goto done; > > + > > + if (fflush(f) == EOF) { > > + err = got_error_from_errno("fflush"); > > + goto done; > > + } > > + if (fseeko(f, 0L, SEEK_SET) == -1) { > > + err = got_error_from_errno("fseek"); > > + goto done; > > + } > > + > > while (*status == GOT_STATUS_MODIFY) { > > linelen = getline(&line, &linesize, f); > > if (linelen == -1) { > > @@ -1537,7 +1574,8 @@ get_modified_file_content_status(unsigned char *status > > break; > > } > > > > - if (strncmp(line, markers[i], strlen(markers[i])) == 0) { > > + if (*line == '+' && > > + strncmp(line + 1, markers[i], strlen(markers[i])) == 0) { > > I suspect using the diff_result structure would be more efficient here. > > It should be possible to skip the unidiff output step entirely by exposing > got_diffreg_result and jumping to relevant lines in the working version of > the file, available in diffreg_result->diff_result's right side. > But we can parse the text diff for now and experiment with diff_result later. Yes, I do agree with this and it's what I wanted to do. Without going into a long story, we had some drama here today with a storm and I didn't get to continue work on this till after dinner tonight so I went for the quicker-to-implement option as I know this impacts the OpenBSD tree so we want a resolution quickly for release. But, yes, I will improve this tomorrow by using diff result :) > > @@ -5018,12 +5064,14 @@ append_ct_diff(struct got_commitable *ct, int *diff_he > > if (diff_staged) { > > if (ct->staged_status != GOT_STATUS_MODIFY && > > ct->staged_status != GOT_STATUS_ADD && > > - ct->staged_status != GOT_STATUS_DELETE) > > + ct->staged_status != GOT_STATUS_DELETE && > > + ct->staged_status != GOT_STATUS_CONFLICT) > > We do not allow STATUS_CONFLICT for staged files. I see no good reason > to allow staging of changes with conflict markers, that would just > complicate things even more. Thanks for clarifying this: I wasn't at all sure about these additional GOT_STATUS_CONFLICT changes but thought better to add them and bring it to your attention so I could both be certain and learn something. > > @@ -5051,6 +5099,7 @@ append_ct_diff(struct got_commitable *ct, int *diff_he > > const char *label1 = NULL, *label2 = NULL; > > switch (ct->staged_status) { > > case GOT_STATUS_MODIFY: > > + case GOT_STATUS_CONFLICT: > > As above, staged_status should only be one of A, D, or M. > We should not allow files in 'C' status to be staged in any way. > > > @@ -5217,7 +5269,8 @@ collect_commitables(void *arg, unsigned char status, > > } > > > > if (staged_status == GOT_STATUS_ADD || > > - staged_status == GOT_STATUS_MODIFY) { > > + staged_status == GOT_STATUS_MODIFY || > > + staged_status == GOT_STATUS_CONFLICT) { > > Same. > > > @@ -5297,7 +5350,8 @@ collect_commitables(void *arg, unsigned char status, > > } > > } > > if (ct->staged_status == GOT_STATUS_ADD || > > - ct->staged_status == GOT_STATUS_MODIFY) { > > + ct->staged_status == GOT_STATUS_MODIFY || > > + ct->staged_status == GOT_STATUS_CONFLICT) { > > Same. > > > @@ -5533,14 +5587,21 @@ match_deleted_or_modified_ct(struct got_commitable **c > > char *ct_name = NULL; > > int path_matches; > > > > + /* > > + * XXX Files with GOT_STATUS_CONFLICT must be allowed else > > + * the caller write_tree() will fail to add a new tree entry > > + * and subsequent diffs of the file will show up empty. > > + */ > > Yes, this is fine. Above XXX comment can be removed. Great, done! > > if (ct->staged_status == GOT_STATUS_NO_CHANGE) { > > if (ct->status != GOT_STATUS_MODIFY && > > ct->status != GOT_STATUS_MODE_CHANGE && > > - ct->status != GOT_STATUS_DELETE) > > + ct->status != GOT_STATUS_DELETE && > > + ct->status != GOT_STATUS_CONFLICT) > > continue; > > } else { > > if (ct->staged_status != GOT_STATUS_MODIFY && > > - ct->staged_status != GOT_STATUS_DELETE) > > + ct->staged_status != GOT_STATUS_DELETE && > > + ct->staged_status != GOT_STATUS_CONFLICT) > > But the rules for commit of staged files should not be changed. > > > continue; > > } > > > > @@ -5744,7 +5805,9 @@ write_tree(struct got_object_id **new_tree_id, int *ne > > /* NB: Deleted entries get dropped here. */ > > if (ct->status == GOT_STATUS_MODIFY || > > ct->status == GOT_STATUS_MODE_CHANGE || > > - ct->staged_status == GOT_STATUS_MODIFY) { > > + ct->status == GOT_STATUS_CONFLICT || > > + ct->staged_status == GOT_STATUS_MODIFY || > > + ct->staged_status == GOT_STATUS_CONFLICT) { > > Same. > > > err = alloc_modified_blob_tree_entry( > > &new_te, te, ct); > > if (err) > > @@ -5804,7 +5867,8 @@ update_fileindex_after_commit(struct got_worktree *wor > > ct->staged_status == GOT_STATUS_DELETE) { > > got_fileindex_entry_remove(fileindex, ie); > > } else if (ct->staged_status == GOT_STATUS_ADD || > > - ct->staged_status == GOT_STATUS_MODIFY) { > > + ct->staged_status == GOT_STATUS_MODIFY || > > + ct->staged_status == GOT_STATUS_CONFLICT) { > > Same. > > > got_fileindex_entry_stage_set(ie, > > GOT_FILEIDX_STAGE_NONE); > > got_fileindex_entry_staged_filetype_set(ie, 0); > > @@ -5948,12 +6012,14 @@ commit_worktree(struct got_object_id **new_commit_id, > > > > /* Blobs for staged files already exist. */ > > if (ct->staged_status == GOT_STATUS_ADD || > > - ct->staged_status == GOT_STATUS_MODIFY) > > + ct->staged_status == GOT_STATUS_MODIFY || > > + ct->staged_status == GOT_STATUS_CONFLICT) > > Same. Ditto for the staged_status changes. Updated diff incorporates all your suggestions. Thanks for the improvements :) diffstat refs/remotes/origin/main refs/heads/main M got/got.1 | 27+ 4- M got/got.c | 68+ 23- M include/got_worktree.h | 4+ 4- M lib/worktree.c | 77+ 16- M regress/cmdline/commit.sh | 73+ 1- M regress/cmdline/update.sh | 1+ 1- 6 files changed, 250 insertions(+), 49 deletions(-) diff refs/remotes/origin/main refs/heads/main commit - f990756a3987ba6410baf611d561e9b8f285f047 commit + b83449a8ed946f9145945d1089f1b40b88fcc2e1 blob - bb1825eecded3623bbabcf0541bce55367696327 blob + b6cc16e8e5a4b02bf33a4f2af21bc0fd0f58c4bd --- got/got.1 +++ got/got.1 @@ -1652,7 +1652,7 @@ is a directory. .Tg ci .It Xo .Cm commit -.Op Fl NnS +.Op Fl CNnS .Op Fl A Ar author .Op Fl F Ar path .Op Fl m Ar message @@ -1740,6 +1740,14 @@ or Git configuration settings. environment variable, or .Xr got.conf 5 , or Git configuration settings. +.It Fl C +Allow committing files in conflicted status. +.Pp +Committing files with conflict markers should generally be avoided. +Cases where conflict markers must be stored in the repository for +some legitimate reason should be very rare. +There are usually ways to avoid storing conflict markers verbatim by +applying appropriate programming tricks. .It Fl F Ar path Use the prepared log message stored in the file found at .Ar path @@ -2209,7 +2217,7 @@ This option cannot be used with .Tg rb .It Xo .Cm rebase -.Op Fl aclX +.Op Fl aCclX .Op Ar branch .Xc .Dl Pq alias: Cm rb @@ -2359,6 +2367,11 @@ If this option is used, no other command-line argument .It Fl a Abort an interrupted rebase operation. If this option is used, no other command-line arguments are allowed. +.It Fl C +Allow a rebase operation to continue with files in conflicted status. +This option should generally be avoided, and can only be used with the +.Fl c +option. .It Fl c Continue an interrupted rebase operation. If this option is used, no other command-line arguments are allowed. @@ -2422,7 +2435,7 @@ None of the other options can be used together with .Tg he .It Xo .Cm histedit -.Op Fl acdeflmX +.Op Fl aCcdeflmX .Op Fl F Ar histedit-script .Op Ar branch .Xc @@ -2597,6 +2610,11 @@ If this option is used, no other command-line argument .It Fl a Abort an interrupted histedit operation. If this option is used, no other command-line arguments are allowed. +.It Fl C +Allow a histedit operation to continue with files in conflicted status. +This option should generally be avoided, and can only be used with the +.Fl c +option. .It Fl c Continue an interrupted histedit operation. If this option is used, no other command-line arguments are allowed. @@ -2749,7 +2767,7 @@ or reverted with .Tg mg .It Xo .Cm merge -.Op Fl acn +.Op Fl aCcn .Op Ar branch .Xc .Dl Pq alias: Cm mg @@ -2864,6 +2882,11 @@ If this option is used, no other command-line argument .It Fl a Abort an interrupted merge operation. If this option is used, no other command-line arguments are allowed. +.It Fl C +Allow a merge operation to continue with files in conflicted status. +This option should generally be avoided, and can only be used with the +.Fl c +option. .It Fl c Continue an interrupted merge operation. If this option is used, no other command-line arguments are allowed. blob - 97e8fcfb43b02a9fb38a2a41964a9a6249b31c2b blob + 438e15cb2486d96840625d284c94c90018d7917b --- got/got.c +++ got/got.c @@ -8776,7 +8776,7 @@ usage_commit(void) __dead static void usage_commit(void) { - fprintf(stderr, "usage: %s commit [-NnS] [-A author] [-F path] " + fprintf(stderr, "usage: %s commit [-CNnS] [-A author] [-F path] " "[-m message] [path ...]\n", getprogname()); exit(1); } @@ -9083,7 +9083,7 @@ cmd_commit(int argc, char *argv[]) char *gitconfig_path = NULL, *editor = NULL, *committer = NULL; int ch, rebase_in_progress, histedit_in_progress, preserve_logmsg = 0; int allow_bad_symlinks = 0, non_interactive = 0, merge_in_progress = 0; - int show_diff = 1; + int show_diff = 1, commit_conflicts = 0; struct got_pathlist_head paths; struct got_reflist_head refs; struct got_reflist_entry *re; @@ -9099,7 +9099,7 @@ cmd_commit(int argc, char *argv[]) err(1, "pledge"); #endif - while ((ch = getopt(argc, argv, "A:F:m:NnS")) != -1) { + while ((ch = getopt(argc, argv, "A:CF:m:NnS")) != -1) { switch (ch) { case 'A': author = optarg; @@ -9107,6 +9107,9 @@ cmd_commit(int argc, char *argv[]) if (error) return error; break; + case 'C': + commit_conflicts = 1; + break; case 'F': if (logmsg != NULL) option_conflict('F', 'm'); @@ -9230,8 +9233,8 @@ cmd_commit(int argc, char *argv[]) } cl_arg.repo_path = got_repo_get_path(repo); error = got_worktree_commit(&id, worktree, &paths, author, committer, - allow_bad_symlinks, show_diff, collect_commit_logmsg, &cl_arg, - print_status, NULL, repo); + allow_bad_symlinks, show_diff, commit_conflicts, + collect_commit_logmsg, &cl_arg, print_status, NULL, repo); if (error) { if (error->code != GOT_ERR_COMMIT_MSG_EMPTY && cl_arg.logmsg_path != NULL) @@ -10368,7 +10371,7 @@ usage_rebase(void) __dead static void usage_rebase(void) { - fprintf(stderr, "usage: %s rebase [-aclX] [branch]\n", getprogname()); + fprintf(stderr, "usage: %s rebase [-aCclX] [branch]\n", getprogname()); exit(1); } @@ -10492,7 +10495,8 @@ rebase_commit(struct got_pathlist_head *merged_paths, rebase_commit(struct got_pathlist_head *merged_paths, struct got_worktree *worktree, struct got_fileindex *fileindex, struct got_reference *tmp_branch, const char *committer, - struct got_object_id *commit_id, struct got_repository *repo) + struct got_object_id *commit_id, int allow_conflict, + struct got_repository *repo) { const struct got_error *error; struct got_commit_object *commit; @@ -10504,7 +10508,7 @@ rebase_commit(struct got_pathlist_head *merged_paths, error = got_worktree_rebase_commit(&new_commit_id, merged_paths, worktree, fileindex, tmp_branch, committer, commit, commit_id, - repo); + allow_conflict, repo); if (error) { if (error->code != GOT_ERR_COMMIT_NO_CHANGES) goto done; @@ -11000,6 +11004,7 @@ cmd_rebase(int argc, char *argv[]) int ch, rebase_in_progress = 0, abort_rebase = 0, continue_rebase = 0; int histedit_in_progress = 0, merge_in_progress = 0; int create_backup = 1, list_backups = 0, delete_backups = 0; + int allow_conflict = 0; struct got_object_id_queue commits; struct got_pathlist_head merged_paths; const struct got_object_id_queue *parent_ids; @@ -11017,11 +11022,14 @@ cmd_rebase(int argc, char *argv[]) err(1, "pledge"); #endif - while ((ch = getopt(argc, argv, "aclX")) != -1) { + while ((ch = getopt(argc, argv, "aCclX")) != -1) { switch (ch) { case 'a': abort_rebase = 1; break; + case 'C': + allow_conflict = 1; + break; case 'c': continue_rebase = 1; break; @@ -11043,6 +11051,8 @@ cmd_rebase(int argc, char *argv[]) if (list_backups) { if (abort_rebase) option_conflict('l', 'a'); + if (allow_conflict) + option_conflict('l', 'C'); if (continue_rebase) option_conflict('l', 'c'); if (delete_backups) @@ -11052,12 +11062,19 @@ cmd_rebase(int argc, char *argv[]) } else if (delete_backups) { if (abort_rebase) option_conflict('X', 'a'); + if (allow_conflict) + option_conflict('X', 'C'); if (continue_rebase) option_conflict('X', 'c'); if (list_backups) option_conflict('l', 'X'); if (argc != 0 && argc != 1) usage_rebase(); + } else if (allow_conflict) { + if (abort_rebase) + option_conflict('C', 'a'); + if (!continue_rebase) + errx(1, "-C option requires -c"); } else { if (abort_rebase && continue_rebase) usage_rebase(); @@ -11177,7 +11194,7 @@ cmd_rebase(int argc, char *argv[]) goto done; error = rebase_commit(NULL, worktree, fileindex, tmp_branch, - committer, resume_commit_id, repo); + committer, resume_commit_id, allow_conflict, repo); if (error) goto done; @@ -11356,7 +11373,7 @@ cmd_rebase(int argc, char *argv[]) } error = rebase_commit(&merged_paths, worktree, fileindex, - tmp_branch, committer, commit_id, repo); + tmp_branch, committer, commit_id, 0, repo); got_pathlist_free(&merged_paths, GOT_PATHLIST_FREE_PATH); if (error) goto done; @@ -11427,7 +11444,7 @@ usage_histedit(void) __dead static void usage_histedit(void) { - fprintf(stderr, "usage: %s histedit [-acdeflmX] [-F histedit-script] " + fprintf(stderr, "usage: %s histedit [-aCcdeflmX] [-F histedit-script] " "[branch]\n", getprogname()); exit(1); } @@ -12167,7 +12184,7 @@ histedit_commit(struct got_pathlist_head *merged_paths histedit_commit(struct got_pathlist_head *merged_paths, struct got_worktree *worktree, struct got_fileindex *fileindex, struct got_reference *tmp_branch, struct got_histedit_list_entry *hle, - const char *committer, struct got_repository *repo) + const char *committer, int allow_conflict, struct got_repository *repo) { const struct got_error *err; struct got_commit_object *commit; @@ -12186,7 +12203,7 @@ histedit_commit(struct got_pathlist_head *merged_paths err = got_worktree_histedit_commit(&new_commit_id, merged_paths, worktree, fileindex, tmp_branch, committer, commit, hle->commit_id, - hle->logmsg, repo); + hle->logmsg, allow_conflict, repo); if (err) { if (err->code != GOT_ERR_COMMIT_NO_CHANGES) goto done; @@ -12271,7 +12288,7 @@ cmd_histedit(int argc, char *argv[]) struct got_update_progress_arg upa; int edit_in_progress = 0, abort_edit = 0, continue_edit = 0; int drop_only = 0, edit_logmsg_only = 0, fold_only = 0, edit_only = 0; - int list_backups = 0, delete_backups = 0; + int allow_conflict = 0, list_backups = 0, delete_backups = 0; const char *edit_script_path = NULL; struct got_object_id_queue commits; struct got_pathlist_head merged_paths; @@ -12292,11 +12309,14 @@ cmd_histedit(int argc, char *argv[]) err(1, "pledge"); #endif - while ((ch = getopt(argc, argv, "acdeF:flmX")) != -1) { + while ((ch = getopt(argc, argv, "aCcdeF:flmX")) != -1) { switch (ch) { case 'a': abort_edit = 1; break; + case 'C': + allow_conflict = 1; + break; case 'c': continue_edit = 1; break; @@ -12330,16 +12350,24 @@ cmd_histedit(int argc, char *argv[]) argc -= optind; argv += optind; + if (abort_edit && allow_conflict) + option_conflict('a', 'C'); if (abort_edit && continue_edit) option_conflict('a', 'c'); + if (edit_script_path && allow_conflict) + option_conflict('F', 'C'); if (edit_script_path && edit_logmsg_only) option_conflict('F', 'm'); if (abort_edit && edit_logmsg_only) option_conflict('a', 'm'); + if (edit_logmsg_only && allow_conflict) + option_conflict('m', 'C'); if (continue_edit && edit_logmsg_only) option_conflict('c', 'm'); if (abort_edit && fold_only) option_conflict('a', 'f'); + if (fold_only && allow_conflict) + option_conflict('f', 'C'); if (continue_edit && fold_only) option_conflict('c', 'f'); if (fold_only && edit_logmsg_only) @@ -12358,6 +12386,8 @@ cmd_histedit(int argc, char *argv[]) option_conflict('f', 'e'); if (drop_only && abort_edit) option_conflict('d', 'a'); + if (drop_only && allow_conflict) + option_conflict('d', 'C'); if (drop_only && continue_edit) option_conflict('d', 'c'); if (drop_only && edit_logmsg_only) @@ -12371,6 +12401,8 @@ cmd_histedit(int argc, char *argv[]) if (list_backups) { if (abort_edit) option_conflict('l', 'a'); + if (allow_conflict) + option_conflict('l', 'C'); if (continue_edit) option_conflict('l', 'c'); if (edit_script_path) @@ -12390,6 +12422,8 @@ cmd_histedit(int argc, char *argv[]) } else if (delete_backups) { if (abort_edit) option_conflict('X', 'a'); + if (allow_conflict) + option_conflict('X', 'C'); if (continue_edit) option_conflict('X', 'c'); if (drop_only) @@ -12406,7 +12440,9 @@ cmd_histedit(int argc, char *argv[]) option_conflict('X', 'l'); if (argc != 0 && argc != 1) usage_histedit(); - } else if (argc != 0) + } else if (allow_conflict && !continue_edit) + errx(1, "-C option requires -c"); + else if (argc != 0) usage_histedit(); /* @@ -12722,7 +12758,7 @@ cmd_histedit(int argc, char *argv[]) if (have_changes) { error = histedit_commit(NULL, worktree, fileindex, tmp_branch, hle, - committer, repo); + committer, allow_conflict, repo); if (error) goto done; } else { @@ -12798,7 +12834,7 @@ cmd_histedit(int argc, char *argv[]) } error = histedit_commit(&merged_paths, worktree, fileindex, - tmp_branch, hle, committer, repo); + tmp_branch, hle, committer, allow_conflict, repo); got_pathlist_free(&merged_paths, GOT_PATHLIST_FREE_PATH); if (error) goto done; @@ -13031,7 +13067,7 @@ usage_merge(void) __dead static void usage_merge(void) { - fprintf(stderr, "usage: %s merge [-acn] [branch]\n", getprogname()); + fprintf(stderr, "usage: %s merge [-aCcn] [branch]\n", getprogname()); exit(1); } @@ -13047,7 +13083,7 @@ cmd_merge(int argc, char *argv[]) struct got_object_id *branch_tip = NULL, *yca_id = NULL; struct got_object_id *wt_branch_tip = NULL; int ch, merge_in_progress = 0, abort_merge = 0, continue_merge = 0; - int interrupt_merge = 0; + int allow_conflict = 0, interrupt_merge = 0; struct got_update_progress_arg upa; struct got_object_id *merge_commit_id = NULL; char *branch_name = NULL; @@ -13061,11 +13097,13 @@ cmd_merge(int argc, char *argv[]) err(1, "pledge"); #endif - while ((ch = getopt(argc, argv, "acn")) != -1) { + while ((ch = getopt(argc, argv, "aCcn")) != -1) { switch (ch) { case 'a': abort_merge = 1; break; + case 'C': + allow_conflict = 1; case 'c': continue_merge = 1; break; @@ -13081,6 +13119,12 @@ cmd_merge(int argc, char *argv[]) argc -= optind; argv += optind; + if (allow_conflict) { + if (abort_merge) + option_conflict('a', 'C'); + if (!continue_merge) + errx(1, "-C option requires -c"); + } if (abort_merge && continue_merge) option_conflict('a', 'c'); if (abort_merge || continue_merge) { @@ -13269,7 +13313,8 @@ cmd_merge(int argc, char *argv[]) } else { error = got_worktree_merge_commit(&merge_commit_id, worktree, fileindex, author, NULL, 1, branch_tip, branch_name, - repo, continue_merge ? print_status : NULL, NULL); + allow_conflict, repo, continue_merge ? print_status : NULL, + NULL); if (error) goto done; error = got_worktree_merge_complete(worktree, fileindex, repo); blob - 4ea02aaea560daeba058d4b8541f5ea222eb7586 blob + f7de090bfb2bd10312bdd0c389b5eabe41a33fdd --- include/got_worktree.h +++ include/got_worktree.h @@ -257,7 +257,7 @@ const struct got_error *got_worktree_commit(struct got */ const struct got_error *got_worktree_commit(struct got_object_id **, struct got_worktree *, struct got_pathlist_head *, const char *, - const char *, int, int, got_worktree_commit_msg_cb, void *, + const char *, int, int, int, got_worktree_commit_msg_cb, void *, got_worktree_status_cb, void *, struct got_repository *); /* Get the path of a commitable worktree item. */ @@ -318,7 +318,7 @@ const struct got_error *got_worktree_rebase_commit(str const struct got_error *got_worktree_rebase_commit(struct got_object_id **, struct got_pathlist_head *, struct got_worktree *, struct got_fileindex *, struct got_reference *, const char *, struct got_commit_object *, - struct got_object_id *, struct got_repository *); + struct got_object_id *, int, struct got_repository *); /* Postpone the rebase operation. Should be called after a merge conflict. */ const struct got_error *got_worktree_rebase_postpone(struct got_worktree *, @@ -392,7 +392,7 @@ const struct got_error *got_worktree_histedit_commit(s const struct got_error *got_worktree_histedit_commit(struct got_object_id **, struct got_pathlist_head *, struct got_worktree *, struct got_fileindex *, struct got_reference *, const char *, struct got_commit_object *, - struct got_object_id *, const char *, struct got_repository *); + struct got_object_id *, const char *, int, struct got_repository *); /* * Record the specified commit as skipped during histedit. @@ -469,7 +469,7 @@ got_worktree_merge_commit(struct got_object_id **new_c struct got_worktree *worktree, struct got_fileindex *fileindex, const char *author, const char *committer, int allow_bad_symlinks, struct got_object_id *branch_tip, const char *branch_name, - struct got_repository *repo, + int allow_conflict, struct got_repository *repo, got_worktree_status_cb status_cb, void *status_arg); /* blob - c4fc2f02d953c285d633a68f2ef9f0cebc512bca blob + 055da929b846e6b27b801620463ae896ca9faf0a --- lib/worktree.c +++ lib/worktree.c @@ -1513,9 +1513,14 @@ done: return err; } -/* Upgrade STATUS_MODIFY to STATUS_CONFLICT if a conflict marker is found. */ +/* + * Upgrade STATUS_MODIFY to STATUS_CONFLICT if a + * conflict marker is found in newly added lines only. + */ static const struct got_error * -get_modified_file_content_status(unsigned char *status, FILE *f) +get_modified_file_content_status(unsigned char *status, + struct got_blob_object *blob, const char *path, struct stat *sb, + FILE *ondisk_file) { const struct got_error *err = NULL; const char *markers[3] = { @@ -1523,11 +1528,46 @@ get_modified_file_content_status(unsigned char *status GOT_DIFF_CONFLICT_MARKER_SEP, GOT_DIFF_CONFLICT_MARKER_END }; + FILE *f = NULL, *f1 = NULL; int i = 0; char *line = NULL; size_t linesize = 0; ssize_t linelen; + off_t sz1 = 0; + if (*status == GOT_STATUS_MODIFY) { + f = got_opentemp(); + if (f == NULL) + return got_error_from_errno("got_opentemp"); + + f1 = got_opentemp(); + if (f1 == NULL) { + err = got_error_from_errno("got_opentemp"); + goto done; + } + + if (blob) { + err = got_object_blob_dump_to_file(&sz1, NULL, NULL, + f1, blob); + if (err) + goto done; + } + + err = got_diff_blob_file(blob, f1, sz1, NULL, ondisk_file, 1, + sb, path, GOT_DIFF_ALGORITHM_MYERS, 0, 0, 0, NULL, f); + if (err) + goto done; + + if (fflush(f) == EOF) { + err = got_error_from_errno("fflush"); + goto done; + } + if (fseeko(f, 0L, SEEK_SET) == -1) { + err = got_error_from_errno("fseek"); + goto done; + } + } + while (*status == GOT_STATUS_MODIFY) { linelen = getline(&line, &linesize, f); if (linelen == -1) { @@ -1537,7 +1577,8 @@ get_modified_file_content_status(unsigned char *status break; } - if (strncmp(line, markers[i], strlen(markers[i])) == 0) { + if (*line == '+' && + strncmp(line + 1, markers[i], strlen(markers[i])) == 0) { if (strcmp(markers[i], GOT_DIFF_CONFLICT_MARKER_END) == 0) *status = GOT_STATUS_CONFLICT; @@ -1545,7 +1586,13 @@ get_modified_file_content_status(unsigned char *status i++; } } + +done: free(line); + if (f != NULL && fclose(f) == EOF && err == NULL) + err = got_error_from_errno("fclose"); + if (f1 != NULL && fclose(f1) == EOF && err == NULL) + err = got_error_from_errno("fclose"); return err; } @@ -1786,7 +1833,8 @@ get_file_status(unsigned char *status, struct stat *sb if (*status == GOT_STATUS_MODIFY) { rewind(f); - err = get_modified_file_content_status(status, f); + err = get_modified_file_content_status(status, blob, ie->path, + sb, f); } else if (xbit_differs(ie, sb->st_mode)) *status = GOT_STATUS_MODE_CHANGE; done: @@ -4947,6 +4995,7 @@ struct collect_commitables_arg { int have_staged_files; int allow_bad_symlinks; int diff_header_shown; + int commit_conflicts; FILE *diff_outfile; FILE *f1; FILE *f2; @@ -5023,7 +5072,8 @@ append_ct_diff(struct got_commitable *ct, int *diff_he } else { if (ct->status != GOT_STATUS_MODIFY && ct->status != GOT_STATUS_ADD && - ct->status != GOT_STATUS_DELETE) + ct->status != GOT_STATUS_DELETE && + ct->status != GOT_STATUS_CONFLICT) return NULL; } @@ -5180,13 +5230,16 @@ collect_commitables(void *arg, unsigned char status, staged_status != GOT_STATUS_DELETE) return NULL; } else { - if (status == GOT_STATUS_CONFLICT) + if (status == GOT_STATUS_CONFLICT && !a->commit_conflicts) { + printf("C %s\n", relpath); return got_error(GOT_ERR_COMMIT_CONFLICT); + } if (status != GOT_STATUS_MODIFY && status != GOT_STATUS_MODE_CHANGE && status != GOT_STATUS_ADD && - status != GOT_STATUS_DELETE) + status != GOT_STATUS_DELETE && + status != GOT_STATUS_CONFLICT) return NULL; } @@ -5536,7 +5589,8 @@ match_deleted_or_modified_ct(struct got_commitable **c if (ct->staged_status == GOT_STATUS_NO_CHANGE) { if (ct->status != GOT_STATUS_MODIFY && ct->status != GOT_STATUS_MODE_CHANGE && - ct->status != GOT_STATUS_DELETE) + ct->status != GOT_STATUS_DELETE && + ct->status != GOT_STATUS_CONFLICT) continue; } else { if (ct->staged_status != GOT_STATUS_MODIFY && @@ -5744,6 +5798,7 @@ write_tree(struct got_object_id **new_tree_id, int *ne /* NB: Deleted entries get dropped here. */ if (ct->status == GOT_STATUS_MODIFY || ct->status == GOT_STATUS_MODE_CHANGE || + ct->status == GOT_STATUS_CONFLICT || ct->staged_status == GOT_STATUS_MODIFY) { err = alloc_modified_blob_tree_entry( &new_te, te, ct); @@ -5953,7 +6008,8 @@ commit_worktree(struct got_object_id **new_commit_id, if (ct->status != GOT_STATUS_ADD && ct->status != GOT_STATUS_MODIFY && - ct->status != GOT_STATUS_MODE_CHANGE) + ct->status != GOT_STATUS_MODE_CHANGE && + ct->status != GOT_STATUS_CONFLICT) continue; if (asprintf(&ondisk_path, "%s/%s", @@ -6104,7 +6160,8 @@ got_worktree_commit(struct got_object_id **new_commit_ got_worktree_commit(struct got_object_id **new_commit_id, struct got_worktree *worktree, struct got_pathlist_head *paths, const char *author, const char *committer, int allow_bad_symlinks, - int show_diff, got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg, + int show_diff, int commit_conflicts, + got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg, got_worktree_status_cb status_cb, void *status_arg, struct got_repository *repo) { @@ -6157,6 +6214,7 @@ got_worktree_commit(struct got_object_id **new_commit_ cc_arg.have_staged_files = have_staged_files; cc_arg.allow_bad_symlinks = allow_bad_symlinks; cc_arg.diff_header_shown = 0; + cc_arg.commit_conflicts = commit_conflicts; if (show_diff) { err = got_opentemp_named(&diff_path, &cc_arg.diff_outfile, GOT_TMPDIR_STR "/got", ".diff"); @@ -6713,7 +6771,7 @@ rebase_commit(struct got_object_id **new_commit_id, struct got_worktree *worktree, struct got_fileindex *fileindex, struct got_reference *tmp_branch, const char *committer, struct got_commit_object *orig_commit, const char *new_logmsg, - struct got_repository *repo) + int allow_conflict, struct got_repository *repo) { const struct got_error *err, *sync_err; struct got_pathlist_head commitable_paths; @@ -6737,6 +6795,7 @@ rebase_commit(struct got_object_id **new_commit_id, cc_arg.worktree = worktree; cc_arg.repo = repo; cc_arg.have_staged_files = 0; + cc_arg.commit_conflicts = allow_conflict; /* * If possible get the status of individual files directly to * avoid crawling the entire work tree once per rebased commit. @@ -6832,7 +6891,8 @@ got_worktree_rebase_commit(struct got_object_id **new_ struct got_pathlist_head *merged_paths, struct got_worktree *worktree, struct got_fileindex *fileindex, struct got_reference *tmp_branch, const char *committer, struct got_commit_object *orig_commit, - struct got_object_id *orig_commit_id, struct got_repository *repo) + struct got_object_id *orig_commit_id, int allow_conflict, + struct got_repository *repo) { const struct got_error *err; char *commit_ref_name; @@ -6856,7 +6916,7 @@ got_worktree_rebase_commit(struct got_object_id **new_ err = rebase_commit(new_commit_id, merged_paths, commit_ref, worktree, fileindex, tmp_branch, committer, orig_commit, - NULL, repo); + NULL, allow_conflict, repo); done: if (commit_ref) got_ref_close(commit_ref); @@ -6871,7 +6931,7 @@ got_worktree_histedit_commit(struct got_object_id **ne struct got_fileindex *fileindex, struct got_reference *tmp_branch, const char *committer, struct got_commit_object *orig_commit, struct got_object_id *orig_commit_id, const char *new_logmsg, - struct got_repository *repo) + int allow_conflict, struct got_repository *repo) { const struct got_error *err; char *commit_ref_name; @@ -6887,7 +6947,7 @@ got_worktree_histedit_commit(struct got_object_id **ne err = rebase_commit(new_commit_id, merged_paths, commit_ref, worktree, fileindex, tmp_branch, committer, orig_commit, - new_logmsg, repo); + new_logmsg, allow_conflict, repo); done: if (commit_ref) got_ref_close(commit_ref); @@ -7852,7 +7912,7 @@ got_worktree_merge_commit(struct got_object_id **new_c struct got_worktree *worktree, struct got_fileindex *fileindex, const char *author, const char *committer, int allow_bad_symlinks, struct got_object_id *branch_tip, const char *branch_name, - struct got_repository *repo, + int allow_conflict, struct got_repository *repo, got_worktree_status_cb status_cb, void *status_arg) { @@ -7898,6 +7958,7 @@ got_worktree_merge_commit(struct got_object_id **new_c cc_arg.repo = repo; cc_arg.have_staged_files = have_staged_files; cc_arg.allow_bad_symlinks = allow_bad_symlinks; + cc_arg.commit_conflicts = allow_conflict; err = worktree_status(worktree, "", fileindex, repo, collect_commitables, &cc_arg, NULL, NULL, 1, 0); if (err) blob - 0543d4bebab658396a069f968553a9b43594ca74 blob + 488cdaf04a738a2e6a11df99d0d5df6b7a604c83 --- regress/cmdline/commit.sh +++ regress/cmdline/commit.sh @@ -294,6 +294,7 @@ test_commit_rejects_conflicted_file() { echo "modified alpha" > $testroot/wt/alpha (cd $testroot/wt && got commit -m "modified alpha" >/dev/null) + local commit_id1=`git_show_head $testroot/repo` (cd $testroot/wt && got update -c $initial_rev > /dev/null) @@ -317,8 +318,14 @@ test_commit_rejects_conflicted_file() { (cd $testroot/wt && got commit -m 'commit it' > $testroot/stdout \ 2> $testroot/stderr) + ret=$? + if [ $ret -eq 0 ]; then + echo "got commit succeeded unexpectedly" + test_done "$testroot" "$ret" + return 1 + fi - echo -n > $testroot/stdout.expected + echo "C alpha" > $testroot/stdout.expected echo "got: cannot commit file in conflicted status" \ > $testroot/stderr.expected @@ -333,7 +340,72 @@ test_commit_rejects_conflicted_file() { ret=$? if [ $ret -ne 0 ]; then diff -u $testroot/stderr.expected $testroot/stderr + test_done "$testroot" "$ret" + return 1 fi + + (cd $testroot/wt && got commit -C -m 'commit it' > $testroot/stdout \ + 2> $testroot/stderr) + ret=$? + if [ $ret -ne 0 ]; then + echo "got commit failed unexpectedly" + test_done "$testroot" "$ret" + return 1 + fi + + # make sure the conflicted commit produces a diff + local conflict_commit=`git_show_head $testroot/repo` + local blob_minus=`got tree -r $testroot/repo -c $commit_id1 -i | \ + grep 'alpha$' | cut -d' ' -f1` + local blob_plus=`got tree -r $testroot/repo -c $conflict_commit -i | \ + grep 'alpha$' | cut -d' ' -f1` + + echo -n > $testroot/stderr.expected + cmp -s $testroot/stderr.expected $testroot/stderr + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stderr.expected $testroot/stderr + test_done "$testroot" "$ret" + return 1 + fi + + (cd $testroot/wt && got diff -c master > $testroot/stdout) + + echo -n > $testroot/stdout.expected + cat > $testroot/stdout.expected <>>>>>> +EOF + + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + test_done "$testroot" "$ret" + return 1 + fi + + (cd $testroot/wt && got status > $testroot/stdout) + + echo -n > $testroot/stdout.expected + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + fi test_done "$testroot" "$ret" } blob - afb98ebb0b2de07c90dfa887d9d20fe0cb5f0db4 blob + 1ab6bfe75bae74735e4040c6d98ea1dea7cd9dc3 --- regress/cmdline/update.sh +++ regress/cmdline/update.sh @@ -2970,7 +2970,7 @@ test_update_binary_file() { fi (cd $testroot/wt && got status > $testroot/stdout) - echo 'C foo' > $testroot/stdout.expected + echo 'M foo' > $testroot/stdout.expected echo -n '? ' >> $testroot/stdout.expected (cd $testroot/wt && ls foo-1-* >> $testroot/stdout.expected) echo -n '? ' >> $testroot/stdout.expected -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68